Austenem/CAT-754 Refactor "My Lists" #3671
Conversation
| dateSaved: Date.now(), | ||
| dateLastModified: Date.now(), |
There was a problem hiding this comment.
Perhaps rename these to saved_timestamp and last_modified_timestamp. I think of a date object or string when thinking of a date.
| async function fetchResponse({ | ||
| url, | ||
| token, | ||
| method, | ||
| body, | ||
| }: { | ||
| url: string; | ||
| token: string; | ||
| method: 'PUT' | 'DELETE'; | ||
| body?: string; | ||
| }) { | ||
| const response = await fetch(url, { | ||
| method, | ||
| headers: { | ||
| Authorization: `Bearer ${token}`, | ||
| 'Content-Type': 'application/json', | ||
| }, | ||
| body, | ||
| }); | ||
|
|
||
| if (!response.ok) { | ||
| console.error('UKV API failed with error: ', await response.text()); | ||
| } | ||
| } |
There was a problem hiding this comment.
This is pattern used elsewhere so no fault in introducing it, but could we use the fetcher in the swr helpers?
| function useSavedLists() { | ||
| const urls = useUkvApiURLs(); | ||
| const { groupsToken, isAuthenticated } = useAppContext(); | ||
| const { setTransferredToProfile } = useSavedListsAlertsStore(); |
There was a problem hiding this comment.
| const { setTransferredToProfile } = useSavedListsAlertsStore(); | |
| const setTransferredToProfile = useSavedListsAlertsStore((state) => setTransferredToProfile); |
We should slice the store when accessing specific fields. This avoids updates when state outside of the field you're picking changes.
https://github.com/pmndrs/zustand?tab=readme-ov-file#selecting-multiple-state-slices
| editListRemote, | ||
| } from 'js/components/savedLists/api'; | ||
|
|
||
| const savedEntitiesSelector = (state: SavedEntitiesStore) => ({ |
There was a problem hiding this comment.
Is this a subset of the store or the entire thing? If it's the entire store we don't need the selector.
| storeHasBeenSetRef.current = true; | ||
| wasLoadingRef.current = isLoading; |
There was a problem hiding this comment.
Do we need both storeHasBeenSetRef and wasLoadingRef, I believe they will always have the same value?
| storeHasBeenSetRef.current = true; | ||
| wasLoadingRef.current = isLoading; | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [isAuthenticated, isLoading]); |
There was a problem hiding this comment.
Is this to avoid putting the store into the dependencies?
| copySavedItemsToRemoteStore({ savedEntitiesLocal, savedListsLocal, urls, groupsToken }); | ||
| store.setEntities(savedEntitiesLocal); | ||
| store.setLists(savedListsLocal); | ||
| } else { | ||
| // If the user already has saved entities and lists in their remote store, we need to copy those over to the local store. | ||
| store.setEntities(savedEntities); | ||
| store.setLists(savedLists); |
There was a problem hiding this comment.
Why are we still using a store for the saved lists/entities. Wouldn't we be fetching this data from the API using `swr1?
Summary
Broad update to the "My Lists" feature across the portal that leverages the new UKV API.
Major updates include:
Design Documentation/Original Tickets
CAT-754 Jira ticket
Testing
Tested using the following checklist (repeated with an authenticated and non-authenticated profile):
Screenshots/Video
Updated Search page
Updated Collections dataset table
Example of banner shown after initial transfer of lists to a user's profile
Checklist
CHANGELOG-your-feature-name-here.mdis present in the root directory, describing the change(s) in full sentences.Additional Notes
The majority of the updated files in this PR are either TypeScript conversions or updates to files that used the
SavedEntitiesStore, as that store is now wrapped in a hook (useSavedLists).